Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: enable core24 plugin tests #5209

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

sergiusens
Copy link
Collaborator

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run tox run -m lint?
  • Have you successfully run tox run -e test-py310? (supported versions: py39, py310, py311, py312)

Issue raised: #5207

Signed-off-by: Sergio Schvezov <[email protected]>
- Remove the dotnet test for core24 from build and run
- Move the dotnet plugin test to the plugin suite

Signed-off-by: Sergio Schvezov <[email protected]>
@sergiusens sergiusens requested review from tigarmo and bepri January 17, 2025 15:45
Copy link
Contributor

@bepri bepri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a lot of movin' around! 20.04 spread test failures seem to be happening on other branches, so likely unrelated.

@sergiusens
Copy link
Collaborator Author

@bepri I still need to move those other python tests!

@bepri bepri self-requested a review January 17, 2025 21:00
spread.yaml Outdated
tests/spread/core24-suites/plugins/:
summary: core24 plugin tests
systems:
- ubuntu-24.04*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do these tests build destructively?
One argument "pro" using something lower than 24.04 is that it lets us catch failures in classic snaps

SNAP/colcon_msg_package: colcon-msg-package

systems:
- ubuntu-22.04
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are using 22.04, is that on purpose?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, but I will drop it from here and make the suite use 22.04

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, they could be handled by the global suite systems filter, which I switched to 22.04, but these tests will not work on non amd64 or arm64, so the filter is more specific

@bepri bepri requested review from tigarmo and removed request for bepri February 13, 2025 14:20
@bepri
Copy link
Contributor

bepri commented Feb 14, 2025

OSV failure is due to #5217, Flutter spread failures due to #5258. Colcon failures are kill timeouts.

@bepri bepri requested a review from lengau February 14, 2025 21:13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably out of scope here, but I'm curious whether:

  1. We shouldn't be updating this to a newer kernel for the core24 stuff
  2. We couldn't disable some more of this kernel config to speed up the time it runs. For example there are device drivers here we probably don't need etc. Basically IMO each config item enabled beyond the bare basics probably deserves its own explanation of what we're testing by enabling it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the "new" kernel plugin is in #4302 and I've heard that a new plugin may be written that replaces the effort in that PR, I'm not entirely convinced of the value of porting this test to core24 (although we may as well keep it since it's already here).

I think @lengau's comments are valid and we should request both points when a new plugin is ready to land on main.

@@ -10,6 +10,7 @@ environment:
PARAM/classic: "--classic"
PARAM/poetry: ""
PARAM/uv: ""
SNAPCRAFT_BUILD_ENVIRONMENT: ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably make a note of why here.

Copy link
Collaborator

@mr-cal mr-cal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, I have 2 observations:

  1. I see many of the task.yamls are still running on 22.04. It's not as important as it used to be when the tests ran in destructive mode, but it is a nice organization method and ideally keeps the spread tests at the same duration across bases. Since you have a systems key in the spread.yaml for the core24-suites/plugins entry, you can drop all the systems in task.yamls unless it's obvious a particular test has special requirements, like it needs to run on 22.04 or on amd64 only.

  2. Can you update the scheduled spread test workflow to include the manual tests? For example, kernel and matter are manual.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the "new" kernel plugin is in #4302 and I've heard that a new plugin may be written that replaces the effort in that PR, I'm not entirely convinced of the value of porting this test to core24 (although we may as well keep it since it's already here).

I think @lengau's comments are valid and we should request both points when a new plugin is ready to land on main.

restore: |
cd "${SNAP}"
snapcraft clean
rm -f ./*.snap
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rm -f ./*.snap
rm -f ./*.snap
rm -rf ./squashfs-root

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants